Annotate DependencyInjection to make it linker friendly - #40227

Tagging subscribers to this area: @eerhardt, @maryamariyan
See info in area-owners.md if you want to be subscribed.

cc @dadhi @alexmg @seesharper @tillig @ENikS @ipjohnson @pakrym

More on this general feature here https://github.com/mono/linker/blob/master/docs/design/reflection-flow.md

Is this for DI libraries or client code? Will it remove client constructors marked for injection?

DI libraries. Today if you link the entire application it will remove constructors use in DI registrations because the linker can't see the which constructors are in use:

var services = new ServiceCollection();
services.AddSingleton<IFoo, Foo>();
var sp = services.BuildServiceProvider();
var foo = sp.GetService<IFoo>();
foo.Bar();

This would fail before this PR because Foo's constructor would be removed after linking. The annotations instruct the linker to preserve constructors of the generic second argument (the implementation type in this case).

But what about constructors marked with [InjectionConstructor] attributes? I am assuming these will be gone as well?
Some classes are annotated instead of registering them and never touched by xx.AddXXX() methods.

@ENikS I don't know if there's a pattern to preserve those. Usually attribute only approaches require assembly scanning, which is also not linker friendly. It's possible that pattern is just incompatible and will result in the user needing to manually preserve those types.

cc @vitek-karas @MichalStrehovsky

Without that MEF, MEF2, Unity and probable some other are not compatible with the pattern.

@davidfowl thank you for heads up!

Without that MEF, MEF2, Unity and probable some other are not compatible with the pattern.

Correct. We haven't gotten to making MEF and MEF2 linker friendly. It isn't scheduled for .NET 5, but it may be included in .NET 6.

@davidfowl Thanks for letting us know about this.

So to preserve the constructors for a given type, the type needs to be mentioned in a method taking a Type parameter (or generic Type parameter )annotated with the DynamicallyAccessedMembers? If so, is that the only way to salvage these types? :)

If so, is that the only way to salvage these types? :)

It's the only way that works with the dataflow analysis within the linker.

The problem is basically that code like:

var  o = Activator.CreateInstance(someType);

may or may not work after trimming, depending on whether the default constructor for someType was preserved. By default, you'll get a warning at the time trimming runs for code like this. The warning will say "hey, there's a piece of reflection that I can't analyze. the code may or may not work at runtime".

You can get rid of that warning four ways:

1. You annotate things with the new attributes like in this pull request so that you'll end up with an annotation on someType that guarantees the default constructor was kept. This is the preferred solution, because it both makes the API "just work" (if linker can reason about the annotated pieces of code), and you got rid of the warning.
2. You suppress the warning with a suppression attribute and ensure the default constructor is kept through other means. E.g. add a direct call to the constructor somewhere. This is not great because by adding a warnings suppression, you're giving up on linker's warnings in the method and it's now your responsibility to make sure the API calls succeed after trimming.
3. You get rid of reflection. E.g. use a source generator.
4. You don't trim and mark the API with a new RequiresUnreferencedCodeAttribute. Linker will warn whenever a trimmed app calls into such API (this is to give a better user experience - instead of user getting a warning about a stray use of reflection in a library, they get a message that the library author made a deliberate choice not to be trimming friendly).

Does RequiresUnreferencedCodeAttribute disable trimming on the assembly/type/member specified?

Does RequiresUnreferencedCodeAttribute disable trimming on the assembly/type/member specified?

No. It is a way to mark an API as unsafe for trimming. This lets the caller know that they should either call a different API or they need to do extra work to make this call safe (e.g. explicitly preserve types/methods/properties/etc).

For example Assembly.Load(string | byte[]) methods will be marked with RequiresUnreferencedCodeAttribute.

You annotate things with the new attributes like in this pull request so that you'll end up with an annotation on someType that guarantees the default constructor was kept. This is the preferred solution, because it both makes the API "just work" (if linker can reason about the annotated pieces of code), and you got rid of the warning.

Does it have to be new attribute? Can you use existing MEF attributes?
These are available on every framework and would spare a lot of refactoring for project owners. We are talking about thousands of types to annotate...

Perhaps you could add an interface, so something like Unity could add to injection attributes as well?

Without support for custom constructor annotations you are cutting out huge chunk of corporate users that use third party IOC containers. Perhaps this could be revisited?

Nothing is being cut, those applications will still work with less aggressive trimming modes, they just won't be able to trim the assemblies completely (or at all).

BTW, this trimming mode isn't the default and we're still a while away from being able to trim our own framework fully. We're just getting started here.

I understand, just trying to make sure this scenario is considered from the beginning and is not an afterthought when it is too late to change

See dotnet/linker#1402

@davidfowl thanks for the heads up.

Can we apply DynamicallyAccessedMembers just to the api surface or do we need to put the attribute everywhere?

Can we apply DynamicallyAccessedMembers just to the api surface or do we need to put the attribute everywhere?

You can apply it to just the API surface but the linker warns if it isn't applied transitively.

Thanks @MichalStrehovsky for such a detailed explanation.👍

Is there any way we can start to test this? Is it possible to enable this using the preview 7 bits?

I've tried dotnet publish -c Release -r osx-x64 --self-contained true -p:PublishSingleFile=false -p:PublishTrimmed=true on a console app , but that did not seems to yield any warnings nor did it trim unreferenced types.

Finally I was just wondering if this is something that is mainly meant for Xamarin apps or do guys you see this as general feature that eventually can be applied to all .Net 5 apps?

@seesharper I made this for you https://github.com/davidfowl/LinkedAspNetCoreApplication. Run it with dotnet publish -r osx-x64

Wow, that was fast. Thanks a lot, David. I'll have a look first thing tomorrow. Again, I gotta hand it to you guys. The way that you involve the community early on new features is just priceless. 👍💪 Really appreciate it